TEP-0075: Validate object name and key name have no dots #5090
Conversation
tekton-robot
added
do-not-merge/release-note-label-needed
tekton-robot
added
release-note
|
The following is the coverage report on the affected files.
|
|
Is |
Yes! string/array param name can still use dots. And using bracket notation is the way to reference them like the example you commented. This PR is to make sure only object param name and its key names do not contain dots. cc @mattmoor |
Thanks for clarifying! 👍 |
tekton-robot
added
the
kind/cleanup
|
/kind misc |
|
The following is the coverage report on the affected files.
|
|
/test tekton-pipeline-unit-tests |
|
/retest |
|
The following is the coverage report on the affected files.
|
|
/retest |
|
@chuangw6 Can we mention this naming limitation for object params in the docs? |
|
/kind misc |
tekton-robot
added
size/L
|
The following is the coverage report on the affected files.
|
|
@abayer: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
The following is the coverage report on the affected files.
|
|
/test check-pr-has-kind-label |
|
@abayer: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/assign @ywluogg |
|
/test pull-tekton-pipeline-alpha-integration-tests |
chuangw6
force-pushed
the
object-name-key-format-validation
branch
from
d067674 to
9a9c70a
Compare
|
The following is the coverage report on the affected files.
|
|
Re: check-pr-has-kind-label: |
| // - Must only contain alphanumeric characters, hyphens (-), underscores (_), and dots (.) | ||
| // - Must begin with a letter or an underscore (_) | ||
| // - Exception: Object param name and key names cannot contain dots (.) | ||
| func validateNameFormat(allParamNames sets.String, objectParams []ParamSpec) (errs *apis.FieldError) { |
There was a problem hiding this comment.
I'm a bit confused why this is not called in validateStepVariables after this regex match has been removed from validateVariables?
There was a problem hiding this comment.
Name format validation should happen before validating its usage. The logic was in that order too before the change.
There was a problem hiding this comment.
Do you mean the naming of the step variables happen before validating its usage? Can you point me to where it is?
There was a problem hiding this comment.
name of parameter names, which is this function you are commenting on.
There was a problem hiding this comment.
Talked to @ywluogg offline. We decided to make the function arguments more explicit i.e. the first argument is only for string&array type since the format requirements for those two types are the same; and the second argument is for object type since it requires not containing dots specifically.
chuangw6
force-pushed
the
object-name-key-format-validation
branch
from
9a9c70a to
c7464bf
Compare
|
The following is the coverage report on the affected files.
|
|
/remove-kind misc |
tekton-robot
removed
the
kind/misc
|
/test check-pr-has-kind-label |
|
@ywluogg: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
chuangw6
force-pushed
the
object-name-key-format-validation
branch
from
c7464bf to
ba212d3
Compare
Prior to this commit, dots are allowed to be used in param names to support domain-scoped names. However, object params have already supported domain-scoped names since it has a list of keys. In addition, using dots in object param names and key names have conflicts. See more details in tektoncd/community#711. In this change, we validate against those object names and key names that contain dots.
chuangw6
force-pushed
the
object-name-key-format-validation
branch
from
ba212d3 to
8122b24
Compare
|
The following is the coverage report on the affected files.
|
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dibyom The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
tekton-robot
added
the
approved
|
/lgtm |
Changes
Prior to this commit, dots are allowed to be used in param names to
support domain-scoped names. However, object params have already
supported domain-scoped names since it has a list of keys. In addition,
using dots in object param names and key names have conflicts.
See more details in tektoncd/community#711.
In this change, we validate against those object names and key names
that contain dots.
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
(if there are no user facing changes, use release note "NONE")
Release Notes